Repository navigation
Make releasing a realtime channel that isn't detached an error (RTS4c-e) - #1251
SimonWoolf wants to merge 11 commits into
Conversation
…e/core-android Applies the first step of PDR-091b to this repo: the shared implementation becomes the new internal core artifacts, published under the io.ably.pubsub group at the new 2.0.0 major. - The `java` module becomes `core`, publishing io.ably.pubsub:core (jar). - The `android` module becomes `core-android`, publishing io.ably.pubsub:core-android (aar). - GROUP moves to io.ably.pubsub and VERSION_NAME to 2.0.0 for everything this repo publishes (liveobjects, pubsub-adapter and network-client-* move group alongside, per the rollout plan). - Both core POMs state explicitly that they are internal implementation artifacts for Ably's own device/server packages, not for direct use. - liveobjects, pubsub-adapter, uts and examples retarget their project dependencies; CI workflows, CONTRIBUTING and the uts README follow the new task paths. The io.ably:ably-java and io.ably:ably-android artifacts are never published from this branch again; 1.x fixes ship from a maintenance branch per PDR-091b. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…2.0.0 The test deliberately hardcodes the expected ably-java/<version> agent value rather than deriving it from Defaults (per its own comment), so it must be bumped with VERSION_NAME. The wire value was already correctly ably-java/2.0.0; only the canary was stale. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the two public artifacts of the PDR-091b split, following the ably-js reference implementation (ably-js#2293): - io.ably.pubsub:server (jar, on :core): PubSubServer.httpClientBuilder() and PubSubServer.realtimeClientBuilder(). - io.ably.pubsub:device (multiplatform Android, JVM): PubSubDevice.clientBuilder(). - A shared side helper (shared/src/main/java, compiled into both door artifacts rather than published) owns the ably-pubsub-device and ably-pubsub-server agent identifiers and the stamping rules: caller entries preserved, side entry applied last and unoverridable, caller's options never mutated, null passing through to the core's own error. The -device/-server suffixes are load-bearing for MAU billing classification and documented as such. - Fixes ClientOptions.copy() to carry headers, fallbackHosts, transportParams and agents, which it previously dropped; the doors rely on copy() for non-mutating stamping. Covered by a new unit test. - Server tests include a wire-level assertion that the Ably-Agent HTTP header carries ably-pubsub-server/<version> alongside the ably-java base identifier; device instrumentation tests assert the same contract and run in the emulator matrix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PDR-091b requires core, core-android, device and server to release together on one version, with partial release impossible: - A verifyReleaseArtifacts task asserts the exact published artifact set, group and lockstep VERSION_NAME (catching module-local overrides), failing before anything is uploaded. The release workflow runs it ahead of publishAndReleaseToMavenCentral. - A release-dry-run job in check.yml runs the pre-flight plus publishToMavenLocal on every PR, so coordinate and version regressions surface continuously rather than on release day. - README gains a note describing the in-development 2.0 artifact set and that 1.x installation instructions still apply. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirrors ably-js#2294: the UTS constructs every client through a single seam (TestRealtimeClient/TestRestClient in ClientFactories.kt), now selected by the uts.side system property — `core` (default) keeps the core constructors; `server` routes both client kinds through PubSubServer's side-stamping builders. The builders only stamp the side-declaring agent flag and pass everything else through (DebugOptions included, via its copy() override), so conformance must be identical on both legs; CI runs both for the UTS unit and integration tiers. A harness self-test (SideModesTest, mirroring ably-js's side_modes.test.ts) asserts each mode's stamp on the wire via the mock HTTP engine — bare versionless flag in server mode, none in core mode — so a broken seam cannot silently degrade the server leg into a duplicate core run. Unlike ably-js there is no device leg: io.ably.pubsub:device is an Android artifact and cannot run on the JVM this suite uses; its stamping contract is covered by the device module's instrumentation tests. Also completes DebugOptions.copy() with the same four fields the base ClientOptions.copy() was missing (headers, fallbackHosts, transportParams, agents) — the door stamping relies on the polymorphic copy() to carry the suite's mock hooks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The server-mode UTS leg surfaced a real platform behavior: realtime rejects a token-authenticated connection that declares the server side through the agent entry alone, with 40167 "a connection or request may only declare itself as a server via a signed x-ably-clientType token claim". The signed-claim mechanism is PDR-091 deferred decision D2 and does not exist yet, so nothing the test infrastructure can mint will authenticate a token-auth server client. Key-auth server clients are unaffected (verified against sandbox), so the two token-auth test classes (TokenRequestTest, AuthReauthTest) now call assumeSideSupportsTokenAuth() and are reported skipped, not failed, on the server leg; the core leg still runs them. When D2 lands, the test infra can mint the claim and the assumption gets deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The UTS spec authenticates this test with a JWT; the Java port had substituted a native TokenRequest for convenience. Restoring the JWT (AblyJwt: HS256 via JDK crypto, no external library) lets the test carry the signed x-ably-clientType=server claim on the server leg — the only server-side declaration realtime accepts on token auth — so it now runs on every leg instead of being skipped. Verified against sandbox: the claim-bearing JWT connects and re-authenticates where the bare agent flag was rejected with 40167. assumeSideSupportsTokenAuth stays for the native-token tests (TokenRequestTest): the native token format cannot carry the claim yet. Also drops DR/ticket numbers from code comments in this PR's files; the behavior is described in place instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ntifier The family identifier shares the ably-pubsub- prefix with the side flags, so the harness self-test now matches the side identifiers exactly (via the Side constants) rather than by prefix, and asserts the renamed family entry is always present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
TokenRequestTest's two clients now sit on opposite sides of the seam, matching how native tokens are really used: the minting client — the createTokenRequest surface under test — goes through the door on every leg, so the server leg exercises token minting through the server package; the consuming client models the device the token was minted for and is always a plain core client, since a client may not authenticate itself with a native token while declaring the server side (realtime rejects that with 40167, and the native token format cannot carry the required signed claim). Nothing is skipped on any leg any more, so assumeSideSupportsTokenAuth is deleted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The device and server doors exposed only no-arg fluent builders, but the branch's own callers — the UTS ClientFactories seam and the device instrumentation test — call realtimeClientBuilder(options), httpClientBuilder(options) and clientBuilder(options). That mismatch broke :uts:compileKotlin and :device:compileDebugAndroidTestJavaWithJavac, and with them every CI job that builds either module. The fluent setters cannot express a pre-assembled DebugOptions (transportFactory, webSocketEngineFactory, clock), so the UTS server leg has to hand the door an options object rather than replay it setter by setter. Each door gains one overload taking ClientOptions, and the builder wraps that object instead of its own empty one, so a DebugOptions subclass and its mock transports reach the client intact. A key or token still goes through the builder's own key()/token() setter: there is no key-string overload. Also drops shared/src/main/java/io/ably/pubsub/internal/Side.java, a second copy of the side-stamping helper that no module compiled — settings.gradle.kts never included it and no source set added it as a srcDir, so io.ably.lib.util.Side is the one the doors actually use. The two references to the dead class, SideModesTest's import and a Defaults javadoc pointer, now name the surviving one. The instrumentation test asserts non-mutation the way PubSubServerTest does, on the caller's agents map rather than on the identity of options.agents, because build() stamps that field in place. :device had no androidTest dependencies at all, so add JUnit, a runner and the testInstrumentationRunner the emulator matrix needs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🟡 Changes recommended
Address concurrency and released-channel callback safety, and add FAILED-state coverage.
2 open findings
What changed in this PR
Implements RTS4c–e for next-major realtime channel release semantics, rejecting release of non-terminal channels with error 90011.
Changes:
- Removes implicit detach-on-release behavior.
- Adds UTS-derived release tests.
- Updates teardown, documentation, and obsolete tests.
| File | Summary |
|---|---|
pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt |
Updates release documentation. |
liveobjects/src/test/kotlin/io/ably/lib/liveobjects/integration/setup/IntegrationTest.kt |
Avoids releasing attached channels during teardown. |
lib/src/test/kotlin/io/ably/lib/uts/unit/realtime/ChannelsCollectionTest.kt |
Adds RTS4c–e release coverage. |
lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java |
Removes obsolete detach-on-release coverage. |
lib/src/main/java/io/ably/lib/realtime/ChannelBase.java |
Removes obsolete released-channel detach handling. |
lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java |
Enforces terminal-state release and LiveObjects disposal. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ChannelState state = channel.state; | ||
| if (state != ChannelState.initialized && state != ChannelState.detached && state != ChannelState.failed) { | ||
| // RTS4e | ||
| throw AblyException.fromErrorInfo(new ErrorInfo( | ||
| "Can only release a channel in a state where there is no possibility of further updates from the server being received (initialized, detached, or failed). The current state is " + state, |
There was a problem hiding this comment.
@coderabbitai can you validate and expand on the above
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd '^(ChannelBase|Channel|InternalMap)\.java$'
fd '^(ChannelBase|Channel|InternalMap)\.java$' --exec ast-grep outline {} --items all
rg -n -C3 --type java '\b(markAsReleased|released|setState|requestState|attach|detach)\b' lib/src/main/java/io/ably/lib/realtime/AblyRealtime.javaLength of output: 8697
🏁 Script executed:
#!/bin/bash
rg -n -C5 'volatile|synchronized|isReleased|released|state =|markAsReleased|checkChannelIsNotReleased' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
ast-grep run --lang java --pattern '$RET $NAME($$$ARGS) { $$$BODY }' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java | head -n 20
rg -n -C25 'void (attachImpl|detachImpl|markAsReleased|checkChannelIsNotReleased|setState|attachWithTimeout)\(' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
cat -n lib/src/main/java/io/ably/lib/util/InternalMap.java
fd '^LiveObjectsPlugin\.java$' --exec ast-grep outline {} --items all
rg -n -C4 --type java 'void dispose\(String|void dispose\(.*channel|new ErrorInfo\(' lib/src/main/java/io/ably/lib/liveobjectsLength of output: 39767
🏁 Script executed:
#!/bin/bash
sed -n '65,102p' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
sed -n '195,233p' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
sed -n '210,254p' lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
sed -n '1670,1690p' lib/src/main/java/io/ably/lib/realtime/ChannelBase.javaLength of output: 6505
@sacOO7 The core finding is valid. I checked the current code. The proposed plugin cleanup rule needs one additional condition.
Why the state check is unsafe
In lib/src/main/java/io/ably/lib/realtime/ChannelBase.java:
stateis notvolatile.setState()writesstateundersynchronized (this)at Line 150.attachWithTimeout()holds the channel monitor and checks the released flag at Line 528.
In AblyRealtime.java, release() reads state without that monitor. It then removes the channel and marks it released.
This sequence is possible:
| Step | Release thread | Attach thread |
|---|---|---|
| 1 | Reads initialized; validation passes. |
|
| 2 | Acquires the channel monitor; the released flag is still false. |
|
| 3 | Changes the state to attaching and sends ATTACH. |
|
| 4 | Removes the channel and marks it released. |
The result violates the release restriction. The channel leaves the collection while an attach is in progress. Subsequent channel messages can reach the “non-existent channel” path.
A stale state read is also possible because the read does not use the writer’s monitor. However, the sequence above proves the race without relying on a stale read.
What the fix must coordinate
Use the channel monitor, not just the collection monitor, for the state check, conditional removal, and markAsReleased().
This gives concurrent attach and release a defined order:
- If attach changes the state first, release sees
attachingand throws error90011. - If release completes first, attach sees the released flag and throws the existing
IllegalStateException.
Making state volatile alone would not fix the check-then-remove race.
Also check the return value of map.remove(channelName, channel). The current code ignores it. If another release already removed that instance, this invocation must not repeat plugin disposal.
Plugin cleanup needs additional coordination
Successful removal is necessary, but it is not sufficient.
dispose(channelName) identifies plugin resources by name. Meanwhile, channels.get(channelName) can create a replacement channel after removal. The replacement constructor calls liveObjectsPlugin.getInstance(name).
Therefore, old-channel cleanup can overlap with replacement-channel initialization. Synchronizing on the old channel does not protect the replacement channel.
The complete fix must either:
- Coordinate removal and plugin disposal with channel creation for the same name, including constructor initialization; or
- Make plugin disposal specific to the old channel’s resource instance.
The lock design must also respect existing lock ordering and callback behavior.
Useful regression cases are attach winning the race, release winning the race, two concurrent releases, and release overlapping with recreation of the same channel name. This validation used source inspection; I did not run tests.
You are interacting with an AI system.
| map.remove(channelName, channel); | ||
| channel.markAsReleased(); | ||
| if (liveObjectsPlugin != null) { | ||
| liveObjectsPlugin.dispose(channelName); |
There was a problem hiding this comment.
@coderabbitai can you validate and expand on the above
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i '^(LiveObjectsPlugin|DefaultRealtimeObject|Channel|InternalMap)\.(java|kt)$'
rg -n -C3 'void dispose\(|fun dispose\(|class DefaultRealtimeObject|class LiveObjectsPlugin' --glob '*.java' --glob '*.kt' .
fd -i '^(LiveObjectsPlugin|DefaultRealtimeObject|Channel|InternalMap)\.(java|kt)$' -x ast-grep outline {}Length of output: 14726
🏁 Script executed:
#!/bin/bash
ast-grep outline liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt --items all
fd '^ChannelBase\.java$' -x ast-grep outline {} --items all
ast-grep outline lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java --items all
nl -ba liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt | sed -n '1,150p'
nl -ba lib/src/main/java/io/ably/lib/liveobjects/LiveObjectsPlugin.java | sed -n '15,68p'
nl -ba lib/src/main/java/io/ably/lib/util/InternalMap.java | sed -n '1,100p'
nl -ba core/src/main/java/io/ably/lib/realtime/Channel.java | sed -n '1,75p'
fd '^ChannelBase\.java$' -x rg -n -C8 'getInstance\(|ChannelBase\(|markAsReleased\(|liveObjectsPlugin|RealtimeObject' {}
nl -ba liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultRealtimeObject.kt | sed -n '315,345p'
nl -ba lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java | sed -n '208,270p'Length of output: 26072
@sacOO7 The race is valid. I checked channel construction, the plugin registry, and object disposal. One detail needs correction: the replacement channel normally reuses the old DefaultRealtimeObject, rather than creating a new object.
How the race occurs
Assume channel C1 owns LiveObjects instance O1.
| Step | Thread A: release(name) |
Thread B: get(name) |
|---|---|---|
| 1 | Removes C1 from the channel map. |
|
| 2 | Constructs replacement channel C2. |
|
| 3 | The constructor calls liveObjectsPlugin.getInstance(name). The plugin still contains O1, so C2.object receives O1. |
|
| 4 | Inserts C2 into the channel map and returns it. |
|
| 5 | Calls dispose(name), which removes and disposes O1. |
The code supports this sequence:
AblyRealtime.java, Line 249–252: channel removal and plugin disposal are separate operations.lib/src/main/java/io/ably/lib/realtime/ChannelBase.java, Line 1685: channel construction retrieves the LiveObjects instance by name.liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt, Line 12–14:getInstance()usescomputeIfAbsent, so it returns the existing instance.- The same plugin file, Line 26–28:
dispose(name)removes whichever instance is registered under that name.
Disposal cancels the incoming-object handler, disposes the object pool and manager, and removes path subscriptions. C2.object still references that disposed instance. This breaks the replacement channel’s LiveObjects functionality; it does not establish that ordinary channel messaging is unusable.
Both registries use ConcurrentHashMap. That protects individual map operations, not the lifecycle across both maps.
What a fix must cover
The simplest approach is to serialize get(name, options) and release(name) with the same lifecycle lock.
- In
get(), acquire the lock before constructingChannel. The constructor already accesses the plugin registry. - In
release(), hold the lock through channel removal,markAsReleased(), and plugin disposal. - Prefer checking for an existing channel before constructing a new channel.
Locking only release(), or only putIfAbsent(), does not prevent this race.
There is also a second race: release() ignores the result of map.remove(name, channel). Two concurrent release calls can capture C1. After the first call removes C1, a replacement can be created. The second call can then fail to remove C1 but still dispose the replacement’s LiveObjects instance. Cleanup should therefore depend on successful removal.
An ownership-based design is another option. However, conditional removal by object identity alone is insufficient here, because C2 can already share O1. That design must also prevent a replacement channel from acquiring the releasing channel’s LiveObjects instance.
A deterministic regression test should pause release after channel-map removal, start a concurrent get(), and check that the replacement retains a live, registered LiveObjects instance after release completes. A second test should cover two concurrent release calls.
This validation is based on source inspection. I did not run tests.
You are interacting with an AI system.
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Seems, there are race conditions we need to take care of as a part of the fix
Spec 6.3.0 replaces RTS4a, under which release() detaches the channel and then removes it, with RTS4c-e: release() of a non-existent channel is a no-op, a channel in the INITIALIZED, DETACHED or FAILED state is removed synchronously, and release() of a channel in any other state throws 90011 and does nothing else. Channels#release now declares AblyException. Users must call channel.detach() and wait for it to complete before releasing. Since a released channel can no longer be mid-detach, drop the released-channel special cases from the detach path, the test of the old behaviour, and the liveobjects test teardown's release of channels that may still be attached (close() already disposes of them). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
01fee79 to
d87239a
Compare
|
@sacOO7 done the check/remove/markAsReleased, that now happen under the channel's monitor. The dispose-by-name race was not introduced by this PR and is independent of it, and looks like the fix needs a lifecycle lock spanning get() plus a plugin API change, so not gonna pick it up in this PR, I'll leave it to you as someone more familiar with the sdk. |
8c8ba9f to
ba47326
Compare
Implements RTS4c–e from spec 6.3.0 (ably/specification#557) for the next major version.
Breaking change:
channels.release(name)no longer detaches the channel implicitly.INITIALIZED,DETACHEDorFAILED, it is removed from the collection beforerelease()returns.release()throwsAblyExceptionwith code 90011 and status 400, and the channel is left unchanged.AblyRealtime.Channels.releasenow declaresthrows AblyException.Migration: call
channel.detach(listener)and wait for it to complete before callingchannels.release(name).Also:
90011 is registered in ably/ably-common#367. The deprecation warning for the current major is in #1250. Reference ably-js PRs: ably/ably-pubsub-js#2322 (deprecation) and ably/ably-pubsub-js#2323 (next major).
🤖 Generated with Claude Code